[BUG]: fix github repository file id escaping#3415
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
deiga
changed the title
deiga
added
the
vNextPatch
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
Have we tested this doesn't break existing IDs? It looks OK, but worth checking. Also should we add this to the next minor release milestone as we can always cherry pick it into a patch if required?
deiga
removed
the
vNextPatch
deiga
force-pushed
the
fix-github_repository_file-id-escaping
branch
from
65ea3b4 to
2598ebe
Compare
|
@stevehipwell Thanks for asking, I ran v6.11.0 locally and then tried to run this code: same failure, since it was already the v0 migration that was failing! |
deiga
added
the
vNextPatch
|
I think we should release this as a patch, considering that the changes in v6.11.1 are blocking upgrading for anyone with colons in |
|
@deiga do you want to rebase this and then we can get it merged in. Once it's been merged we can decide if we want to cherry pick it onto a patch branch. |
deiga
force-pushed
the
fix-github_repository_file-id-escaping
branch
from
7cb06fa to
8f0bfe8
Compare
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
…capred ID parts Signed-off-by: Timo Sand <timo.sand@f-secure.com>
- We want to ensure that users never accidentally get `hashicorp/github` - `~/` failed to expand on macOS Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
deiga
force-pushed
the
fix-github_repository_file-id-escaping
branch
from
aa309fd to
4ea73fb
Compare
|
@stevehipwell Implemented your requested changes and I commented out the unit test to propose this format again, since I needed them to determine that my code changes didn't break anything |
stevehipwell
left a comment
stevehipwell
left a comment
There was a problem hiding this comment.
LGTM
Let's get the dev.tfrc file removed in a separate PR.
|
@robert-crandall Could you please review this? :) |
There was a problem hiding this comment.
Pull request overview
These provider review instructions are being used.
This PR fixes a regression introduced in v6.11.1 where github_repository_file rejects file paths containing a literal colon. The provider's buildID(repo, file, branch) call refuses non-final parts that contain the : separator; the fix wraps the file/filePath argument in escapeIDPart(...) everywhere the resource ID is built (create, update, import, and the v0→v1 state migration), and adds a real, executable migration test plus an acceptance test exercising a colon-containing file name. Unrelated edits touch examples/resources/repository_file/example_2.tf, the rendered docs/resources/repository_file.md, and examples/dev.tfrc.
Changes:
- Escape the
filesegment withescapeIDPartin all fourbuildIDcall sites on the resource and its migration. - Re-enable the previously commented-out v0→v1 state-upgrade test, switch it to
mustGitHubClient, and add amigrates_with_colon_in_file_pathcase. - Add an acceptance test verifying the resource ID contains the escaped file path and that the
fileattribute round-trips the literal colon.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| github/resource_github_repository_file.go | Wraps file/filePath in escapeIDPart for create, update, and import ID construction. |
| github/resource_github_repository_file_migration.go | Hardens type assertions, escapes filePath when building the migrated ID, tags errors as "state upgrade v0". |
| github/resource_github_repository_file_migration_test.go | Uncomments and updates the upgrade test; adds a colon-in-path case. |
| github/resource_github_repository_file_test.go | New acceptance test asserting ID escaping and file round-trip for a colon-containing path. |
| examples/resources/repository_file/example_2.tf | Replaces autocreate_branch = true with an explicit github_branch resource. |
| docs/resources/repository_file.md | Mirrors the example_2.tf change in generated docs. |
| examples/dev.tfrc | Uses $HOME and adds a direct.exclude for hashicorp/github. |
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
Signed-off-by: Timo Sand <timo.sand@f-secure.com>
|
@stevehipwell copilot caught a bug, please re-review :) |
Resolves #3335
Before the change?
filePathingithub_repository_resourcecontained a colongithub_repository_file#3175 when I changed the ID generation codeAfter the change?
filePathingithub_repository_resourcecontains a colonPull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!